Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix display_document params in noautocomplete mode #826

Merged
merged 2 commits into from
Jan 1, 2024
Merged

Fix display_document params in noautocomplete mode #826

merged 2 commits into from
Jan 1, 2024

Conversation

tompng
Copy link
Member

@tompng tompng commented Dec 28, 2023 •
edited
Loading

Fix destructuring completion params in display_document (only used in noautocomplete mode)

# Line 250
Reline.completion_proc = ->(target, preposing, postposing) {
  ...
  @completion_params = [preposing, target, postposing, bind]
  ...
}

# Line 296
def show_doc_dialog_proc
  doc_namespace = ->(matched) {
    preposing, _target, postposing, bind = @completion_params
    ...
  }
  ...
end

# Line 422
def display_document(matched, driver: nil)
  ...
  _target, preposing, postposing, bind = @completion_params
  # Should be `preposing, _target, postposing, bind = @completion_params`
  ...
end

This bug only affects irb --noautocomplete --type-completor because RegexpCompletor does not use preposing.

$ irb --noautocomplete --type-completor
(type 1.ti[TAB][TAB])
irb(main):001> 1.ti[TAB]
irb(main):001> 1.times[TAB]
(should show rdoc but nothing happens. this pull request fixes it)

@tompng tompng added the bug Something isn't working label Dec 29, 2023
st0012
st0012 approved these changes Dec 29, 2023
View reviewed changes
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍
Should we also add yamatanooroti tests for --noautocomplete cases?

@tompng
Copy link
Member Author

tompng commented Dec 29, 2023

It needs type-completor to test in rendering test.
Instead, I added arguments assertion in display_dialog test

@st0012
Copy link
Member

st0012 commented Jan 1, 2024 •
edited
Loading

I'd avoid mutating internal states for tests as they're bound with deep implementation details.
How about we extract the doc namespace logic to a method and share it between show_doc_dialog_proc and display_document? Since show_doc_dialog_proc is tested, we can also have the coverage on the logic.

diff --git a/lib/irb/input-method.rb b/lib/irb/input-method.rb
index b74974b..573b6da 100644
--- a/lib/irb/input-method.rb
+++ b/lib/irb/input-method.rb
@@ -291,11 +291,12 @@ module IRB
       @auto_indent_proc = block
     end
 
+    def retrieve_doc_namespace(matched)
+      preposing, _target, postposing, bind = @completion_params
+      @completor.doc_namespace(preposing, matched, postposing, bind: bind)
+    end
+
     def show_doc_dialog_proc
-      doc_namespace = ->(matched) {
-        preposing, _target, postposing, bind = @completion_params
-        @completor.doc_namespace(preposing, matched, postposing, bind: bind)
-      }
       ->() {
         dialog.trap_key = nil
         alt_d = [
@@ -311,7 +312,7 @@ module IRB
         cursor_pos_to_render, result, pointer, autocomplete_dialog = context.pop(4)
         return nil if result.nil? or pointer.nil? or pointer < 0
 
-        name = doc_namespace.call(result[pointer])
+        name = retrieve_doc_namespace(result[pointer])
         # Use first one because document dialog does not support multiple namespaces.
         name = name.first if name.is_a?(Array)
 
@@ -419,8 +420,7 @@ module IRB
         return
       end
 
-      _target, preposing, postposing, bind = @completion_params
-      namespace = @completor.doc_namespace(preposing, matched, postposing, bind: bind)
+      namespace = retrieve_doc_namespace(matched)
       return unless namespace
 
       driver ||= RDoc::RI::Driver.new

tompng added 2 commits January 1, 2024 21:32
@tompng
Fix display_document params in noautocomplete mode
e958398
@tompng
Fix wrong preposing and target order in display_document
29205ac 
The fixed wrong-ordered value is not used in RegexpCompletor, so this change does not affect the test.
@tompng tompng force-pushed the fix_display_document_params branch from 276758b to 29205ac Compare January 1, 2024 12:35
@tompng
Copy link
Member Author

tompng commented Jan 1, 2024

The suggested code looks better. Thanks👍
I've updated

@tompng tompng merged commit 08208ad into ruby:master Jan 1, 2024
28 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Jan 1, 2024
@tompng @matzbot
[ruby/irb] Fix display_document params in noautocomplete mode
c0e3c3b 
(ruby/irb#826)

* Fix display_document params in noautocomplete mode

* Fix wrong preposing and target order in display_document

The fixed wrong-ordered value is not used in RegexpCompletor, so this change does not affect the test.

ruby/irb@08208adb5e
@tompng tompng deleted the fix_display_document_params branch January 1, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@st0012 st0012 st0012 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tompng @st0012